Fix/completed handler pwstr result - #36

…ng families (#1)

* test(generator): regenerate the goldens with -update

Every generator test carried a commented-out os.WriteFile loop for refreshing
its golden, so updating them meant an edit-run-revert cycle across seven files.
That is enough friction to make hand-editing the generated output look like the
cheaper way to fix a bug -- and it is not, because the next regeneration
silently reverts it.

    go test ./generator -update    # then read the diff, then run without it

The goldens are embedded with go:embed, so a run that rewrites them is still
comparing against the previous build's copies; -update therefore writes and
returns rather than asserting.

* feat(generator): add an offline regeneration command

update_version_mapping.go can only regenerate as a side effect of checking
Microsoft's release-notes page for a NEW version: it needs the network, it
rewrites the tree in place, and it does nothing at all if the pinned version is
current. None of that suits the one thing a generator change needs, which is to
regenerate from a pinned IDL and diff the result against the committed files.

    go run ./regen -idl WebView2.1.0.2903.40.idl -out /tmp/baseline
    diff -r /tmp/baseline ../pkg/webview2

Doing this is how the drift between the committed generator and the committed
output became measurable rather than suspected.

* build(generator): format the generated output

The generator wrote raw template output, yet the committed pkg/webview2 is
gofmt-clean -- so its 306 files were being formatted by hand after every
regeneration. The measurable cost: ~180 of them differ from a fresh generation
by nothing but import order and blank lines, which is more than enough noise to
hide a real change in a regeneration diff. Hiding real changes in regeneration
diffs is how this package came to carry 800+ hand-patched call sites.

Second benefit, unlooked-for: format.Source rejects invalid Go, so a template
that emits something unparseable now fails the generator and names the file,
instead of writing it out to fail later at go build.

Only the goldens' formatting changes here; no generated logic moves.

* fix(generator): marshal arguments and results the way the ABI defines them

Five families, all in the same two places -- Param.processVtableCallInput and the
method template -- and all with the same failure mode: the generated code
compiles, links, runs, and returns S_OK while corrupting exactly one argument.
Counts are call sites in the pinned 1.0.2903.40 IDL.

1. By-value scalars, ~90 sites. The type switch tested p.Type, the IDL type
   ("BOOL", "INT32", "double"), against Go type names in lower case. It
   therefore matched almost nothing, and every by-value in-parameter fell
   through to the &address catch-all, so the callee read a pointer as an
   integer. p.GoType is the mapped Go type and is what the comparisons meant.
   One wrong field name; no scalar in-parameter in the whole binding was passed
   correctly. Since Go forbids uintptr(someBool), BOOL now goes through a
   boolToUintptr helper -- which subsumes the hand-written branch added for
   PutShouldDetectMonitorScaleChanges, one of the 39 bool sites.

   That same missing pointer guard also broke four OUT parameters, because "int"
   is spelled the same in the IDL and in Go: GetStatusCode and GetExitCode
   passed the value of an uninitialised local where the callee wanted somewhere
   to write, so they returned 0 or an error every time.

2. Handle typedefs and register-sized aggregates, 76 sites. These reached the
   &address catch-all too. The Win32 x64 rule is not "aggregates go by
   reference": an aggregate of exactly 1, 2, 4 or 8 bytes is passed IN A
   REGISTER as an integer of that width, and only the rest go by address. So
   POINT (8 bytes) and RECT (16) take opposite forms, which is what made a
   single &address default look plausible.

   EventRegistrationToken is 61 of the 76. It is struct{ int64 }, so every
   remove_* method in the binding handed the callee the ADDRESS of the token it
   was supposed to match -- no event handler could ever be removed, and remove_
   still returned S_OK.

   maps.go now classifies each type explicitly and the generator FAILS on a type
   in neither table, rather than guessing. Guessing is what produced all of the
   above.

3. String out-parameters, 109 sites. LPWSTR out-params are declared LPWSTR*: the
   callee writes a string pointer into storage we own, so it needs the address
   of our local *uint16. Passing the local's nil value instead gave the callee a
   null to write through, so every string getter returned "" -- with S_OK.

4. ComProc.Call's Errno returned as error. Call's third result is a
   syscall.Errno, which is NON-NIL on success ("The operation completed
   successfully"), so returning it made every successful call look like a
   failure. HRESULT is the real status and is already checked. Upstream fixed
   this across the output by hand in "fix: com error handling" and "fix:
   mischanged error values" but never in the template, so any regeneration
   reintroduced it; that sweep also over-applied in two places, replacing
   UTF16PtrFromString's genuine error with nil in GetHeader. Both are correct
   now. IUnknown::Release's refcount was being discarded the same way, and
   CallRelease returns uint32 accordingly.

5. Callback parameter widths, 3 sites -- the family PR wailsapp#36 patched in the
   output. A callback reached through syscall.NewCallback may not declare a
   parameter wider than a uintptr, and NewCallback enforces that when the
   callback is CONSTRUCTED, which happens in a package-level var initialiser. So
   three CompletedHandlers declaring their LPCWSTR result as a Go string (a
   16-byte header) made merely importing pkg/webview2 panic during init on
   amd64, whether or not the program used them:

       panic: compileCallback: argument size is larger than uintptr

   An outbound method may legitimately take a string, because it converts before
   the call; an inbound one has no such step, so Param.AsCallbackType keeps the
   two apart.

Deliberately not fixed: float in-parameters. On Windows x64 a floating-point
argument goes in XMM0-XMM3 and syscall.LazyProc.Call fills only the integer
registers, so uintptr(f) truncates and &f is equally wrong. There is no
marshalling answer; it needs a different call mechanism. PutZoomFactor is the
live example, and the code says so rather than pretending.

Also still wrong, and out of scope: array-valued parameters have no
representation here at all. GetAllowedOrigins returns *string for an LPWSTR**
array and SetAllowedOrigins takes one string for an array, and
Get/SetCustomSchemeRegistrations has the same shape. Fixing that needs a slice
concept and an ownership decision, not a marshalling change.

* fix(generator): derived vtables include their inherited slots

A COM vtable is flat: a derived interface's vtable begins with its ENTIRE base
chain and only then its own methods. Every derived vtable was generated as
IUnknownVtbl plus that interface's own methods, so each method sat too early by
however many methods the chain above it declares, and a call landed on whichever
unrelated function occupies that offset. It cannot fail loudly -- the wrong slot
holds a real function pointer, so it runs and returns S_OK.

ICoreWebView2_14's AddServerCertificateErrorDetected dispatched at slot 4, which
is ICoreWebView2::get_Settings. get_Settings takes one out-parameter, so it duly
wrote the Settings pointer over the caller's event-handler struct and reported
success. Its correct slot is 107. Registration therefore "succeeded", registered
nothing, and the event never fired -- which on our side meant a TLS certificate
pin that could not engage on any Windows machine.

88 interfaces were affected. Base-interface calls were always correct, which is
exactly why Navigate and AddNavigationCompleted worked throughout and hid it.

The IDL states the base for every interface and the parser already captured it;
it was simply never used here. A first-generation interface's base IS IUnknown,
so one expression covers both cases -- and taking the base from the IDL rather
than from the version suffix matters: ICoreWebView2EnvironmentOptions2 through 8
each derive from IUnknown, not from their predecessor, so inferring the chain
from the numbering gets those seven wrong.

Second, related: the QueryInterface accessors. QueryInterface asks an OBJECT for
another of its interfaces, so an accessor belongs on an interface of the object
that can answer it -- which the declared chain's ROOT names, not the immediate
base. The template put all of them on ICoreWebView2. For the ICoreWebView2_N
chain that is correct, because it is all one object and a caller should not have
to walk thirteen accessors to reach _14. For every other chain it is useless:
GetICoreWebView2Controller2 hung off ICoreWebView2, which is a different object
and can only fail, while ICoreWebView2Controller, which can answer, had no
accessor at all.

So of the 82 accessors, 56 now root on their own object (Environment, Settings,
Profile, Frame, Controller, CompositionController and the event-args chains) and
26 stay on ICoreWebView2. 31 of the 56 were already correct in the committed
output because they had been moved by hand, so a regeneration relocates the
remaining 25 -- and breaks no existing caller.

An earlier draft of this rooted on the immediate base instead. That is also sound
COM, but it forces the walk and it did break callers of GetICoreWebView2_14,
including ours.

Interfaces whose base is IUnknown keep their ICoreWebView2 receiver: there is no
sibling interface to reach them from, and re-rooting them onto IUnknown would
move ~170 methods onto it and delete accessors that already ship, which is an API
break rather than a bug fix.

* test(generator): property tests for each defect family

Six properties of the whole generated binding, checked against the pinned IDL,
rather than goldens for one interface. Every one of them fails on main and passes
after this series.

A golden records what one interface looked like on the day it was written. These
say what must be true of all 306 files, which is the assertion that was missing:
each fixed family was wrong across dozens of interfaces while the golden for
ICoreWebView2_3 sat there passing.

  TestVtableEmbedsDeclaredBase              every derived vtable embeds its
                                            declared base's vtable, so no method
                                            can sit at the wrong slot
  TestQueryInterfaceAccessorReceiver        accessors hang off the object that can
                                            answer the QueryInterface, i.e. the
                                            declared chain's root
  TestCallbackParamsFitInAUintptr           no callback declares a parameter wider
                                            than a uintptr -- i.e. the package can
                                            be imported at all
  TestCallErrnoIsNeverReturnedAsError       Call's Errno is never returned as error
  TestByValueArgumentsAreNotPassedByAddress table-driven per parameter shape: BOOL,
                                            UINT32, HWND, 8-byte token, POINT,
                                            COLOR, RECT, in/out pointers, in/out
                                            strings
  TestGeneratedOutputIsFormatted            the output is a gofmt fixpoint

The vtable test asserts embedding rather than computed slot numbers on purpose:
embedding the immediate base is sufficient by induction, whereas a test that
recomputed the offsets would be checking its own arithmetic against itself.

* chore(webview2): regenerate from the pinned IDL

    cd scripts && go run ./regen -idl WebView2.1.0.2903.40.idl -out ../pkg/webview2

No hand edits. That is the point of the commit: before this series the committed
pkg/webview2 was NOT the output of the committed generator, so a regeneration
would have reverted 800+ corrections that only ever existed in the output.

108 files change, and every changed line belongs to a family fixed earlier in the
series:

     61  EventRegistrationToken passed by value    (32 files)  remove_* now works
     54  BOOL passed by value                     (36 files)
     51  handle typedefs and integers by value     (25 files)
     25  QI accessors rooted on their own object   (25 files)
      9  POINT passed by value                      (4 files)
      8  vtable embeds its declared base            (8 files)
      6  int out-parameters passed by address       (6 files)  Get{Status,Exit}Code
      4  UTF16PtrFromString's error kept            (2 files)  GetHeader

Verified: GOOS=windows GOARCH=amd64 go build ./... and GOARCH=arm64
go build ./pkg/..., go vet ./pkg/webview2 clean, the generator's own tests pass,
and the Zentinel Windows GUI that consumes this package builds against this tree
unchanged -- checked through a go.work override, since a pseudo-version in go.mod
silently resolves to the module cache instead.

Not verified: none of this has been exercised on Windows hardware in this exact
form. The vtable, cert-pin and external-link paths were verified on a real machine
against the equivalent hand-patched tree, which is where these fixes came from.
The by-value corrections -- remove_*, POINT, the handle typedefs -- are new, and
so far rest on the calling convention rather than on a run.

* fix(generator): apply the findings from a review of this series

Reviewed by three independent passes over the preceding seven commits. One commit
rather than several because the fixes overlap in the same two functions, and
file-level boundaries would have misrepresented which change belongs where.

## Regressions this series introduced

1. `defaultErrorValue` tested `uintptrTypedef[p.Type]`, and `p.Type` carries no
   indirection -- so `[out] HANDLE**` returned the integer 0 for a `*HANDLE`,
   which does not compile. The `p.GoType == "HANDLE"` test it replaced was false
   for a double pointer and fell through to `nil`. The pointer case now comes
   first, which is the right order on its own terms: for `*T` the only thing that
   matters is that it is a pointer.

2. `Library.Process` pre-indexes interfaces with the comment "nothing guarantees
   a base is declared first", but enums were still registered as each one was
   processed, while `Param.IsEnum()` is consulted while processing an INTERFACE.
   An enum declared after its user therefore looked like an unknown type -- which
   used to mean a silently wrong &address and, after this series, means the
   generator stops with advice that does not apply. Same bug class, half fixed.
   Both are indexed in one pre-pass now.

## Claims this series made that were false

3. "Interfaces whose base is IUnknown keep the ICoreWebView2 receiver they have
   today" -- they do not, and never did. The accessor is guarded by `BaseClass`,
   which `generateVtbl` blanks for `IUnknown`, so those 169 interfaces get no
   accessor at all. The `{{else}}` arm that supposedly served them was
   unreachable, as was the matching fallback in the test. The statement appeared
   in three places including a test comment; all three now say what happens.

4. "Float in-parameters have no marshalling answer; LazyProc.Call can only fill
   the integer registers." Wrong. `runtime/sys_windows_amd64.s` copies each of
   the first four argument slots into the matching XMM register, with a comment
   saying it does so precisely "in case any of the arguments are floating point
   values". So the bit pattern IS the argument, and `math.Float64bits` produces
   it. The scope was understated too: 12 methods take a `double`, not one, and
   every one was handing the callee the ADDRESS of a Go float reinterpreted as a
   double -- so a zoom factor was a denormal or ~1e-300.

   windows/arm64 is genuinely unsolved: `sys_windows_arm64.s` loads R0-R7 and
   never V0-V7, with a TODO to do what amd64 does. Passing the bits in an integer
   register is no worse there than passing a pointer was, so this is a strict
   improvement on both, and the comment now says which arch is which.

## Gaps in the families this series claimed to fix

5. The `*` and LPWSTR branches gained a direction check and the `**` branch did
   not. An IN parameter is already the `T**` the caller built, so taking its
   address hands the callee a `T***`: it reads our local's own value as the first
   element and calls through it. `CreateObjectCollection` is the live example, and
   it is a wild call rather than a wrong value.

6. `INT`/`UINT`/`int` mapped to Go's `int`/`uint`, which are 64-bit. Seven
   out-parameters declare a local of the mapped type and hand over its address,
   so a 64-bit local took a 32-bit write and kept its zeroed high half: the sign
   never extended. `GetExitCode` returned 3221225477 for an exit code of
   -1073741819, and `GetKeyEventLParam` is wrong on every key-up, because WM_KEYUP
   sets lParam bit 31. Lowercase `int` -- the IDL's spelling for six of the seven
   -- was absent from the map entirely. Seven public signatures change from
   `int`/`uint` to `int32`/`uint32`; they returned wrong values, so there is
   nothing to preserve.

## Pre-existing defects found on the way

7. A struct field's `BOOL` is 4 bytes and was generated as Go's 1-byte `bool`, so
   `COREWEBVIEW2_PHYSICAL_KEY_STATUS` came out 12 bytes against a native 24. Its
   only use is `GetPhysicalKeyStatus`, which hands WebView2 the address of that
   12-byte local -- so every call wrote 12 bytes past the end of a heap object,
   into whatever shared its size class. The bytes that landed inside were misread
   too: the last three flags sit at native offsets 12/16/20 while Go looked at
   9/10/11, so they were permanently false. Struct fields now have their own type
   map: a parameter's BOOL is converted at the boundary, so Go's `bool` is a free
   kindness there, but a struct field has no boundary and its width is
   load-bearing.

8. An enumerator with no initialiser is PREVIOUS + 1 in C; the generator used its
   ordinal position. That is the same answer only for an enum which either sets no
   values or sets them to their own positions -- true of every shipped WebView2
   enum, which is why it never showed. "A = 5, B, C" produced 5, 1, 2. Values are
   computed numerically where the initialiser is a literal the grammar accepts, so
   the output for every shipped enum is byte-identical; where it cannot be
   evaluated the previous enumerator is named instead, which Go allows.

9. `AddRef` was generated for all 252 interfaces and `Release` for none, so every
   caller of an accessor leaked a reference with nothing to call. Now generated,
   additively, except on handler interfaces -- those are objects we implement, so
   their lifetime is the Go object's and calling through the vtable would re-enter
   our own impl. Relatedly, the accessors discard the HRESULT, which cannot change
   without altering 82 signatures; that is now documented where it is generated,
   including that this series makes it MORE reachable, because the 56 accessors it
   re-rooted were previously on an object that could never answer them.

10. `InterfaceDeclaration.Process` broke out of its loop on finding `Invoke`, so a
    method declared after it was never processed and would be emitted with no
    parameters. `IDL.Generate` returned inside its loop over libraries, so only
    the first was ever generated. Both are unreachable with today's inputs; both
    are gone.

## Tooling and tests

11. `log.Fatalf` in the generator calls `os.Exit`, and it is reachable from the
    generator's own tests -- so reintroducing a bug killed the test binary mid-run
    with no attributable failure, and every test after it silently never ran.
    Errors are returned and reported now.

12. `TestGeneratedOutputIsFormatted` could not fail for the reason it claimed: its
    input had already been through `gofmtAll`, so it asserted that formatting
    formatted content is a no-op, and deliberately mangling a template left it
    green. Replaced with `TestCommittedOutputMatchesGenerator`, which asserts the
    one invariant this whole arrangement exists to establish and which nothing
    checked -- that the committed `pkg/webview2` IS the generator's output.
    Verified to go red when a committed file is hand-edited.

13. The anti-vacuity thresholds were counts of today's IDL with two units of
    headroom, so one interface silently ceasing to generate a file would have been
    caught by neither the skipped inner assertions nor the floor. They are exact
    completeness assertions now, derived from the same rule the generator uses.
    The callback-width test also missed floats, which `NewCallback` rejects with
    its own panic, and the test's chain walk lacked the cycle guard the production
    code had just gained.

14. `regen` did not clear its output directory, so reusing one across two IDL
    versions left files only the earlier one produced and made `diff -r` read as
    though they were still generated -- defeating the one thing the command is
    for. `Taskfile.yml`'s `gofmt` task over `pkg/webview2` is deleted: formatting
    is the generator's job as of this series, and leaving the task implied
    otherwise.

## Reverted from this series

A hard failure when an interface's base is not declared in the same library. That
is a legitimate input: `com.tmpl` hand-writes `IUnknown`, `IStream` and
`IDataObject` precisely so interfaces can derive from types no IDL declares, the
IDL carries forward declarations, and this generator's own fixtures are
single-interface fragments whose base is absent by construction. The cost is that
a genuinely missing base surfaces as `undefined: <Base>Vtbl` in the consuming
build; distinguishing that from "hand-written elsewhere" needs a registry of what
com.tmpl defines, which is a worse coupling than the deferred error.

## Not fixed, deliberately

- Array-valued parameters still have no representation (`GetAllowedOrigins`
  returns `*string` for an `LPWSTR**`). Dereferencing the result builds a string
  header from a pointer array, so it is an unbounded read, not a wrong value. The
  honest fix is to stop emitting these four methods or return an opaque type, and
  both are API decisions rather than marshalling ones.
- `CoTaskMemFree` is skipped when a method returns early on a failed HRESULT. Only
  a callee that allocates and then fails leaks, which is a contract violation, and
  the local is nil in that case anyway.
- `!= windows.S_OK` rejects other COM success codes; `S_FALSE` appears nowhere in
  any of the six IDLs.
- Enum constants are untyped, so `type X uint32` buys no type safety.

* chore(webview2): regenerate after the review fixes

    cd scripts && go run ./regen -idl WebView2.1.0.2903.40.idl -out ../pkg/webview2

No hand edits, and TestCommittedOutputMatchesGenerator now enforces that.

170 files change. The bulk is additive -- a Release method on each of the 170
non-handler interfaces -- and the rest is the corrections in the previous commit:

    12  doubles passed by value rather than by address
     7  32-bit out-parameters given 32-bit locals
     6  COREWEBVIEW2_PHYSICAL_KEY_STATUS fields at native width (12 -> 24 bytes)
     1  T** in-parameter no longer passed as T***
    82  QueryInterface accessors documented as nil-on-old-runtime

Verified: windows amd64, arm64 and 386 build; go vet clean on pkg/webview2; the
generator's tests pass, including the new committed-equals-generated invariant;
all six IDLs in scripts/ regenerate and the three older ones type-check; and the
Zentinel Windows GUI and tray build against this tree through a go.work override.

* test(webview2): execute the marshalling instead of arguing about it

Every correctness claim in this series -- mine and three reviewers' -- has been an
argument about the Windows x64 calling convention. None of it had been run. That
is precisely how eight families of marshalling bug survived here in the first
place: the wrong code compiles, links, runs and returns S_OK, so reading it was
the only check there was, and reading it is what failed.

A COM object is a pointer to a table of function pointers, so one can be built
entirely out of Go: fill a generated Vtbl with NewComProc(someGoFunc) and hand its
address to the generated wrapper. The wrapper marshals exactly as it would for
real WebView2, and the fake callee sees what actually arrived.

The part worth noticing is what this does NOT need: no WebView2 Runtime, no Edge
install, no display, no network, no elevation. Windows and nothing else -- so it
runs on a stock windows-latest runner, which is the difference between "we
reasoned carefully" and "we know". There is no CI in this repo yet; when there is,
this is the job that matters.

Nine tests, one per family, each asserting the thing the pre-fix code got wrong:

    BOOL in-param arrives as 0/1, not as an address
    EventRegistrationToken arrives as its 8 bytes, so remove_* can match it
    POINT arrives by value with X in the low half
    a double arrives as its bits, observable because Go's syscall asm copies each
      of the first four argument slots into both the integer and the XMM register
    an int32 out-param keeps its sign (-1073741819, not 3221225477)
    a string out-param is written through OUR storage, not a null
    a string in-param arrives as the *uint16 we converted
    COREWEBVIEW2_PHYSICAL_KEY_STATUS is 24 bytes and its flags read at the native
      offsets -- the test writes a full native-layout struct, so a regression to
      the 12-byte version corrupts memory or returns false flags
    HWND arrives by value
    the HRESULT decides the error, and S_OK yields a nil one

Also added, in the generator's own suite: TestVtableDeclaresEveryDeclaredMethod.
Embedding the base vtable gives correct offsets only if each interface also
contributes exactly its own methods in order; the second half of that was asserted
in a commit message and never checked. If the parser ever dropped a method, every
later slot would shift and the failure would look exactly like the bug this series
fixed, while every embedding assertion still passed. Verified: 252 interfaces, one
embedded base and one ComProc per declared method, no exceptions.

Two `go vet` findings remain in the new test file, both in the CoTaskMemAlloc
helper. A string out-parameter must be freed by CoTaskMemFree, so the test has to
hand back COM-allocated memory rather than Go memory, x/sys exposes CoTaskMemFree
but no typed CoTaskMemAlloc, and a syscall returns uintptr -- so one uintptr to
unsafe.Pointer conversion is unavoidable. The memory is owned by the COM allocator
and never moves, which is why the pattern is safe; vet says "possible" because it
cannot know that. Every callback takes typed pointer arguments specifically so
that this is the only place it happens.

* test(webview2): assert the string out-param without a COM allocator

Writing a real string back meant CoTaskMemAlloc, because the generated cleanup
frees it with CoTaskMemFree and giving that a Go pointer corrupts the COM heap.
Reading a syscall's returned address back into an unsafe.Pointer is the one
pattern go vet cannot verify, so the file failed the vet step -- the suite added
to catch silent breakage would have arrived broken.

What that family got wrong is that the callee received the local's nil VALUE
instead of its address, and asserting that needs no allocation: CoTaskMemFree(nil)
and UTF16PtrToString(nil) are both defined no-ops, so the path still runs end to
end. The conversion is library code rather than generated code, and the real
round-trip is covered in the in-parameter direction.

* Create ci.yml

* Update ci.yml

* Update ci.yml

* Update ci.yml

---------

Co-authored-by: goutham.r <goutham.r@zohocorp.com>